[Local catalog] Analytics for sync, launch, and loading #16345
Conversation
- Simplify syncFailed event to automatically extract error details - Map catalog_too_large to sync_skipped reason (not error) - Add comprehensive ineligibility reason mapping for skip events - Remove unnecessary categorizeError helper method
- Track splashScreenErrorShown when error view appears - Track splashScreenRetryTapped when retry button is pressed - Only track for initialCatalogSyncError type (splash screen errors) - Completes analytics implementation for local catalog feature 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
- Track sync started with connection type for incremental syncs - Track sync completed with duration and counts for incremental syncs - Track sync failed for incremental syncs (cancellation and errors) - Ensures pull-to-refresh sync events are tracked via performSmartSync 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
- Created MockAnalytics conforming to Analytics protocol for Yosemite tests - Added 3 focused tests verifying analytics integration: * Full sync tracks started and completed events * Full sync failure tracks error_type property (network_error) * Incremental sync tracks started and completed events - Tests verify event names and key properties without over-specifying 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
joshheald
added
type: task
joshheald
changed the base branch from
trunk
to
feat/WOOMOB-1173-background-catalog-download-updated
Generated by 🚫 Danger |
|
|
…nto feat/WOOMOB-1173-parse-catalog-downloads-in-the-background
joshheald
changed the base branch from
feat/WOOMOB-1173-background-catalog-download-updated
to
feat/WOOMOB-1173-parse-catalog-downloads-in-the-background
…und-old-2' into feat/woomob-1104-local-catalog-analytics
…und' into feat/woomob-1104-local-catalog-analytics
iamgabrielma
left a comment
iamgabrielma
left a comment
There was a problem hiding this comment.
Works well!
✅ pos_local_catalog_downloading_screen_shown
✅ pos_local_catalog_downloading_screen_exit_pos_tapped
✅ local_catalog_stale_warning_shown
✅ local_catalog_stale_warning_dismissed
✅ pos_local_catalog_sync_started
[connection_type: wifi, sync_type: full]
[sync_type: incremental, connection_type: wifi]
✅ pos_local_catalog_sync_completed
[sync_duration_ms: 108584, products_synced: 4976, variations_synced: 12458, total_variations: 12458, sync_type: full, total_products: 4976]
[total_variations: 12458, total_products: 4976, sync_type: incremental, products_synced: 0, sync_duration_ms: 2001, variations_synced: 0]
✅ pos_local_catalog_sync_failed
[error_type: unexpected_error, sync_type: full, error_domain: Yosemite.POSCatalogSyncError, error_code: 5, error_description: Yosemite.POSCatalogSyncError.requestCancelled]
✅ pos_local_catalog_sync_skipped
[reason: catalog_not_stale]
[reason: catalog_too_large]
I did not test the following 2 cases:
- pos_splash_screen_error_shown
- pos_splash_screen_retry_tapped
| } | ||
| .background(Color.posSurface) | ||
| .onAppear { | ||
| if isCatalogSyncing { |
There was a problem hiding this comment.
Should we do this on the view's init to avoid tracking multiple events on potential view recreation?
| ] | ||
|
|
||
| // Apply prefix if: (POS mode is active AND event is in the list) OR event is a local catalog event | ||
| guard (Self.isPOSModeActive && pointOfSaleEventList.contains(event)) || localCatalogEventList.contains(event) else { |
There was a problem hiding this comment.
Good catch, when decorating pos events we've always considered that POS mode would be active...until now. Each time we deal with tracking events makes me wonder of the scalability of what we have.
There was a problem hiding this comment.
Yeah. Even just finding and remembering to add them to the list is a pain, tbh...
There was a problem hiding this comment.
A bunch of these seem to fallback to network_error, do we plan to specialize them further later on? Otherwise perhaps can be simplified, ie: classifyURLError differentiates between 4 cases to just log network_error anyway, so:
private static func classifyURLError(_ error: URLError) -> String {
switch error.code {
case .timedOut:
return "network_timeout"
default:
return "network_error"
}
}
| return WooAnalyticsEvent(statName: .orderDetailWaitingTimeLoaded, properties: [Keys.waitingTime: elapsedTime]) | ||
| return WooAnalyticsEvent( | ||
| statName: .orderDetailWaitingTimeLoaded, | ||
| properties: [Keys.waitingTime: elapsedTime].merging(typedAdditionalProperties) { $1 }) |
There was a problem hiding this comment.
All of them seem to be using waiting time + additional props, we could declare this at the top:
let props = [Keys.waitingTime: elapsedTime]
.merging(additionalProperties.mapValues { $0 as WooAnalyticsEventPropertyType }) { $1 }
switch scenario {
case .orderDetails:
return WooAnalyticsEvent(
statName: .orderDetailWaitingTimeLoaded,
properties: props)
And update the case for pointOfSaleLoaded
| } | ||
| // Track sync failed analytics | ||
| trackAnalytics(WooAnalyticsEvent.LocalCatalog.syncFailed( | ||
| syncType: "full", |
There was a problem hiding this comment.
Maybe is worth to make an enum for the syncType we use in analytics
| } | ||
| } catch { | ||
| DDLogError("⛔️ POSCatalogSyncCoordinator: Failed to get storage counts: \(error)") | ||
| return (0, 0) |
There was a problem hiding this comment.
Nit, just for readability
| return (0, 0) | |
| return (products: 0, variations: 0) |
| // Map ineligibility reasons to skip reasons | ||
| switch reason { | ||
| case .posTabNotEligible: | ||
| return "pos_inactive" |
There was a problem hiding this comment.
Consider extending or moving this to POSLocalCatalogIneligibleReason
extension POSLocalCatalogIneligibleReason {
var skipReason: String {
switch self {
case .posTabNotEligible:
return "pos_inactive"
case .featureFlagDisabled:
...
| grdbManager: ServiceLocator.grdbManager, | ||
| catalogEligibilityChecker: eligibilityService | ||
| catalogEligibilityChecker: eligibilityService, | ||
| siteSettings: nil, |
There was a problem hiding this comment.
siteSettings is currently only set internally on POSCatalogSyncCoordinator, consider if we need to expose it or we can keep it private.
There was a problem hiding this comment.
We also use it in tests. We could make a new internal init for that but it seems pretty clunky. We can perhaps just rely on the default nil to omit it here though.
|
Forgot to add some notes from testing: The local catalog settings tab does not seem to show if there's an error syncing, which disallows me to retry and click on the full sync button if this happens. It could be coincidence as well, since the tab is a bit finicky and does not always show at the moment.
I couldn't trigger this one to show the splash screen when log in/out, neither uninstalling the app. Not specifically related to the event itself. |
Yeah, I'll add a new ticket for this. Because it's essentially settings screen UI I've not really done anything about it yet, but it's at risk of getting missed.
No worries, I've tested them and they work. Couldn't have registered them otherwise! It's easier with a large catalog store and overriding the max catalog size, but you have to wait a moment so that you're deemed eligible for local catalog, so there's a slight balance. With <1000 products and fast network it's very difficult to hit it. Thanks for the other points... I'll take a look before merging. |
Consolidates switch cases that differentiate errors but return the same value, making the code more maintainable without losing functionality. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Extracts common property merging logic to avoid repeating the same pattern across all switch cases. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
- Add POSCatalogSyncType enum to replace string literals - Add skipReason computed property to POSLocalCatalogIneligibleReason - Improve tuple readability with named parameters - Centralize ineligibility reason mapping to avoid duplication 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Relies on the default parameter value for cleaner call site. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Merge after #16342
Description
This PR adds analytics for sync, launch, and loading with the local catalog.
See pdfdoF-84z-p2 for full details. Note that the checkout events will be tackled in WOOMOB-1686
Events
I've tested using WPcom and site credentials connections, which can cause differences in error types
Test Steps
Some of these will be easier to test on a large store, e.g. largefuntesting.mystagingwebsite.com. To use this, set POSLocalCatalogEligibilityService.Constants.defaultCatalogSizeLimit to 100000.
When you need to see the splash screen events, log out first, then log in, as the splash screen is only shown for the first successful sync.
You can force a sync to fail by repeatedly failing a single page of products – set a network breakpoint on
/wc/v3/products, then fail one particular page for it's initial attempt, and all 3 backed-off retries. Use a 500 or similar.You can simulate database errors by editing the
grdbManager.databaseConnection.writeblocks inPOSCatalogPersistenceService, and throwing errors there, e.g.throw DatabaseError(resultCode: .SQLITE_FULL)Screenshots
RELEASE-NOTES.txtif necessary.